[draft] Dialect Conversion without Rollback #93412

matthias-springer · 2024-05-26T13:07:41Z

This commit adds a dialect conversion driver without rollback: OneShotDialectConversionDriver

The new driver reuses some functionality of the greedy pattern rewrite driver. Just a proof of concept, code is not polished yet.

OneShotConversionPatternRewriter is a rewriter that materializes all IR changes immediately.

Adapted two tests to show what kind of changes are needed: nvgpu-to-nvvm and complex-to-standard

Dinistro

IIUC, this inherits from ConversionPatternRewriter to ensure that the conversion patterns can be used with the new OneShotConversionPatternRewriter. While this makes sense from a functional perspective, the introduced inheritance relation seems a bit conflicting, as the super class has supposedly more functionality than the derived class. In many practical cases, replacing usages of ConversionPatternRewriter with a OneShotCovnersionPatternRewriter might work, this is not true in the general case, right?

matthias-springer · 2024-05-27T11:51:12Z

The final OneShotConversionPatternRewriter will support everything that ConversionPatternRewriter supports. A few helper functions are still missing in this PR:

applySignatureConversion
convertRegionTypes
convertNonEntryRegionTypes
replaceUsesOfBlockArgument (removed by [mlir][Transforms] Support replaceAllUsesWith in dialect conversion #84725)

These functions would have to become virtual, so that they can be overridden in OneShotConversionPatternRewriter.

Dinistro · 2024-05-27T15:16:55Z

The final OneShotConversionPatternRewriter will support everything that ConversionPatternRewriter supports. A few helper functions are still missing in this PR:

applySignatureConversion

convertRegionTypes

convertNonEntryRegionTypes

replaceUsesOfBlockArgument (removed by [mlir][Transforms] Support replaceAllUsesWith in dialect conversion #84725)

These functions would have to become virtual, so that they can be overridden in OneShotConversionPatternRewriter.

I was mainly referring to the rollback logic. A pattern using the super class might assume rollback, which then breaks when it uses the specialised class. This violates the liskov substitution principle, which might be considered problematic.
I understand that we might deliberately do so, to ensure a smooth transition to the new infra, without requiring substantial changes.

I just fear that we will never be able to remove the inheritance if we build this in now.

Note: From this standpoint, swapping their position in the inheritance hierarchy might be cleaner (just theoretically).

matthias-springer · 2024-05-27T15:32:44Z

You are right, there is one thing that conversion patterns are no longer allowed to do. From the RFC:

One change is needed for existing conversion patterns: they must not modify the IR if they
return failure. (Same as ordinary rewrite patterns.) Most patterns should already be written in
such a way. for all other patterns, that should be a simple fix.

We already have assertions that check this in the greedy pattern rewrite driver (ExpensiveChecks); must be enabled explicitly because they are expensive. This prototype is reuses some of the greedy pattern rewrite driver, so we get these checks for free.

Swapping the order of the two conversion rewriters in the class hierarchy won't make a difference here: for patterns to work with the new driver, they must not make changes to the IR and then return failure. Also, if ConversionRewriter inherits from OneShotConversionRewriter, it won't be possible to use existing patterns with the new driver (unless we also change the name of ConversionRewriter->LegacyConversionRewriter and OneShotConversionRewriter->ConversionRewriter.)

I just fear that we will never be able to remove the inheritance if we build this in now.

I share some of that concern. But I find it important to build something that is backwards compatible (with existing conversion patterns); with a clear and simple migration path. Otherwise, nobody is going to use the new driver. (There are so many helpers like function op conversion patterns or structural SCF conversion patterns, that people will want to reuse.)

ftynse · 2024-05-28T15:13:04Z

mlir/lib/Transforms/Utils/DialectConversion.cpp

+  return impl->currentTypeConverter;
+}
+
+LogicalResult ConversionPatternRewriter::getAdapterOperands(


Nit: can we agree on Adapter/Adaptor spelling? E.g., we already have OpAdaptor.

ftynse · 2024-05-28T15:14:39Z

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp

                                      const FrozenRewritePatternSet &patterns,
                                      const GreedyRewriteConfig &config);

  /// Add the given operation to the worklist.
  void addSingleOpToWorklist(Operation *op);

  /// Add the given operation and its ancestors to the worklist.
-  void addToWorklist(Operation *op);
+  virtual void addToWorklist(Operation *op);


Do we have an estimate of how much overhead adding a vtable to this class introduces?

We can avoid this virtual function call by templatizing the driver class.

ftynse · 2024-05-28T15:15:50Z

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp

+  void startOpModification(Operation *op) override {
+    PatternRewriter::startOpModification(op);
+  }
+
+  void finalizeOpModification(Operation *op) override {
+    PatternRewriter::finalizeOpModification(op);
+  }
+
+  void cancelOpModification(Operation *op) override {
+    PatternRewriter::cancelOpModification(op);
+  }


Would these still be necessary after the old driver is removed?

The only one that would still be necessary is replaceOp because it must insert unrealized_conversion_casts. All the others could be removed (because the intermediate ConversionPatternRewriter class would be gone).

ftynse · 2024-05-28T15:17:37Z

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp

+
+  /// A cache for unrealized_conversion_casts. To ensure that identical casts
+  /// are not built multiple times.
+  DenseMap<std::pair<Value, Type>, Value> castCache;


Hmm, is it possible that the same original value is casted to multiple different types within the same conversion? Type converter is currently unaware of the surrounding context, so it's unclear to me how that could happen.

I think this can happen during getAdapterOperands. Each conversion pattern can have its own type converter. (That's why there is currentTypeConverter etc.)

func.func @foo(%arg0: i1) { "op1"(%arg0) "op2"(%arg0) }

Let's assume that there is no conversion pattern for func.func. I.e., the block argument is not converted. We could have two different patterns for "op1" and "op2" with different type converters that convert i1 to different types. Now we have to insert two unrealized_conversion_casts with different result types.

(This probably does not happen very often.)

joker-eph · 2024-05-28T17:33:19Z

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp

+  return castOp.getOutputs()[0];
+}
+
+class ConversionPatternRewriteDriver : public GreedyPatternRewriteDriver {


Relying on the greedy driver makes me concerned about the "one shot" aspect.
I would really like to see a simple implementation, with the minimum amount of bookkeeping (ideally not even a worklist!), and relying on the GreedyPatternRewriteDriver does not really go in this direction right now.

"One Shot" may be a misnomer. I really meant "no rollback".

I am reluctant to build something that is not compatible with the large number of existing conversion patterns. I would rather gradually improve the situation in way where users have a painless way to migrate to the new driver. Otherwise, is anybody going to use the new infrastructure?

What's the main issue that you see with a worklist-based approach? Imo, the main complexity of the dialect conversion is because of rollback and late materialization. I never had issues issues debugging a greedy pattern rewrite with -debug so far.

I don't have problem with dialect conversion complexity actually, other than replaceAllUsesWith not doing what it says it does, and the fact that you have to go through the rewriter for these. But you're not changing these aspects are you?

I am reluctant to build something that is not compatible with the large number of existing conversion patterns.

Without rollback you're incompatible anyway, people can't "just adopt it". But otherwise what kind of incompatibility are you worried about here?

Greedy is a fixed-point iterative algorithm which has non-trivial cost associated with it. I don't quite get why it is suitable here?

I don't have problem with dialect conversion complexity actually, other than replaceAllUsesWith not doing what it says it does

I tried to fix that here. It sounded like the rollback logic was getting too complex. (I would agree with that.)

Without rollback you're incompatible anyway, people can't "just adopt it".

I expect that most dialect conversions do not actually need the rollback. I am not aware of any passes in MLIR that require it. (But users may have their own passes that rely on it, that's why I asked in the RFC.) So my hope is that the existing patterns just work with the new driver. I only tried it with NVGPUToNVVM.cpp and ComplexToStandard.cpp so far, and have to try migrating a few more passes to get a better picture.

But you are right, it is not a general replacement. That's why I would keep both drivers side-by-side for a while, so that there is time to handle the tricky cases (if any). (And patterns can be updated gradually. And they can be updated in such a way that they stay compatible with both drivers.)

Greedy is a fixed-point iterative algorithm which has non-trivial cost associated with it. I don't quite get why it is suitable here?

Part of the answer is that I wanted to build something that is compatible. And that requires us to maintain some sort of worklist or resort to (unbounded) recursion, as the current implementation does.

There are some differences to a greedy fixed-point rewrite. (The following are properties of the current and the "new" dialect conversion.)

Ops are always processed from top to bottom.

Patterns are applied only to illegal root ops.

A pattern must either remove the illegal root op or in-place modify it. (We may be able to tighten the rules to require that an in-place modification must make the op legal. That would gives us a stronger guarantee about the worst-case number of pattern applications.)

New illegal ops are allowed to be created, that's why the worklist is needed. The second property is an important one: we do not put an op onto the worklist just because something changed in the vicinity of the op (like in a greedy pattern rewrite).

So I wouldn't call it a greedy rewrite. Jacques asked how this is different from equipping the greedy pattern rewrite driver with a legality function. That got me thinking: large parts of the greedy driver can be reused, only the condition when to put ops on the worklist is different. The only reason why this implementation is in GreedyPatternRewriteDriver.cpp is to safe a few lines code. (And I would eventually rename this file to WorklistBasedPatternDrivers.cpp etc.)

This commit adds a dialect conversion driver without rollback: `OneShotDialectConversionDriver` The new driver reuses some functionality of the greedy pattern rewrite driver. Just a proof of concept, code is not polished yet. `OneShotConversionPatternRewriter` is a rewriter that materializes all IR changes immediately.

matthias-springer · 2025-08-18T08:24:53Z

Subsumed by #151865.

Dinistro reviewed May 27, 2024

View reviewed changes

ftynse reviewed May 28, 2024

View reviewed changes

joker-eph reviewed May 28, 2024

View reviewed changes

Base automatically changed from users/matthias-springer/greedy_pattern_composition to main June 8, 2024 08:26

matthias-springer force-pushed the users/matthias-springer/one_shot_dialect_conv branch from 11dfecd to 6c04a06 Compare June 8, 2024 10:28

matthias-springer closed this Aug 18, 2025

[draft] Dialect Conversion without Rollback #93412

[draft] Dialect Conversion without Rollback #93412

Uh oh!

Conversation

matthias-springer commented May 26, 2024

Uh oh!

Dinistro left a comment

Choose a reason for hiding this comment

Uh oh!

matthias-springer commented May 27, 2024

Uh oh!

Dinistro commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthias-springer commented May 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthias-springer May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthias-springer commented Aug 18, 2025

Uh oh!

Uh oh!

Dinistro commented May 27, 2024 •

edited

Loading

matthias-springer May 29, 2024 •

edited

Loading